Add internal profiling to debug tool #32423
Conversation
SangJunBak
force-pushed
the
jun/debug-tool/add-internal-profiling
branch
3 times, most recently
from
754036d to
a2867a8
Compare
| @@ -1,7 +1,7 @@ | |||
| [package] | |||
| name = "mz-debug" | |||
| description = "Debug tool for self-managed Materialize." | |||
| version = "0.1.0" | |||
| version = "0.2.0" | |||
There was a problem hiding this comment.
bumping the version like this is a good habit, but out of curiosity is it necessary? do we publish it anywhere?
There was a problem hiding this comment.
Not necessary, just something manual I do whenever I decide to publish haha. We do publish it as a curlable binary that we expose via the docs!
There was a problem hiding this comment.
And technically one can download old versions, though we use the version via a git tag, so it never actually reads this cargo.toml. We have instructions to update it here /Users/sangjunbak/materialize/ci/deploy_mz-debug/README.md but kinda upto the developer to make sure both are in sync 😬
| static INTERNAL_HOST_ADDRESS: &str = "127.0.0.1"; | ||
| static INTERNAL_HTTP_PORT: i32 = 6878; | ||
|
|
||
| /// A struct that handles downloading and saving profile data from HTTP endpoints |
There was a problem hiding this comment.
nit: we always try to end comments with periods, in other words, make them complete sentences.
| output_path: &Path, | ||
| ) -> Result<(), anyhow::Error> { | ||
| // Try HTTPS first, then fall back to HTTP if that fails | ||
| let mut url = format!("https://{}", relative_url); |
There was a problem hiding this comment.
Instead of manually formatting the URL like this I would opt to use the url crate and then the set_scheme(...) method on the Url type
There was a problem hiding this comment.
So I think in order to create a URL, we need to use Url::parse which expects an absolute URL. Converted it into a URL anyways via Url::parse(&format!("https://{}", relative_url)) then set the scheme to http later but lemme know if there's a better way!
|
|
||
| /// Downloads and saves heap profile data | ||
| pub async fn dump_heap_profile(&self, relative_url: &str, service_name: &str) -> Result<()> { | ||
| let output_dir = self.context.base_path.join("profiles"); |
There was a problem hiding this comment.
Maybe these directory names should be constants that are grouped at the top of the file? That way it's easier to determine what folders the debug tool might create? Not blocking but just a thought
There was a problem hiding this comment.
Ah like saving profiles or self.context.base_path.join("profiles")? I think saving paths like profiles in static str variables like RELATIVE_DIR makes total sense.
The latter might be an issue since base_path has format mz-debug_<DateTime> where DateTime gets computed on main at the start of the program. We could have it done as a constant then have internal_http_dumper and everything alike refer to it, but I like the flexibility of having the DateTime be computed on runtime in case we:
a) Do multiple runs with multiple date times (i.e. mz-debug/<DateTime>)
b) We do some initialization that takes a while and wanna compute DateTime later
- Instead of waiting until port forwarding, we read from stderr until we know for sure port forwarding has started - Renamed `create_kubectl_port_forwarder` to `create_pg_wire_port_forwarder` since we'll be port forwarding more than once - Removed the retry mechanism given we don't see the port forwarding disconnecting too often and we can't do it AND initial acknowledgement together easily - Tie child process to a struct instead of a tokio spawn.
We create an initialization function that preprocesses as much as possible and maps args to context values.
I kept the trait but realized the enum is a bit redundant, especially since we have the Context enum
Given we're always creating a PathBuf from start time, it makes sense to just pass the PathBuf itself
- Use stdout instead of stderr - Remove sharing of context - Fix bug where process was being killed on connecting to port forward
- Introduced InternalHttpDumpClient for downloading and saving heap profile data and Prometheus metrics from internal HTTP endpoints. - Changes service for port forwarding from balancerd to environmentd for better consistency with metrics - Refactor "find" methods for port forwarding to be more modular. Separates out port finding / port forwarder creation from service finding
- Introduced a new asynchronous function `get_container_ip` to fetch the IP address of a Docker container using its ID. Doing this means we don't need the user to map ports when doing `DOCKER RUN` - Changes mz_connection_url for the emulator to an optional argument and global now that we have the container's IP
- Update CLI flags in docs and clippy/lint errors
SangJunBak
force-pushed
the
jun/debug-tool/add-internal-profiling
branch
from
8a7573a to
a79040c
Compare
- Introduced constants for directory paths in `docker_dumper.rs`, `internal_http_dumper.rs`, and `system_catalog_dumper.rs` for readability - Updated the output file naming convention for heap profiles in `internal_http_dumper.rs` to use a `.gz` extension since they're really gzip files
- Added `url` crate for URL parsing. - Updated warn messages in to print context for improved debugging
See commit messages for details
Motivation
"Scrape prometheus metrics" and "Scrape profiling" from https://github.com/MaterializeInc/database-issues/issues/8908
Tips for reviewer
Checklist
$T ⇔ Proto$Tmapping (possibly in a backwards-incompatible way), then it is tagged with aT-protolabel.